-
-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom help #50
Custom help #50
Conversation
Adds a Settings struct for creating parsers/commands with NewParserWithSettings or NewCommandWithSettings. Settings HelpDisabled - defaults false, set true to not generate a help argument for parser/command HelpSname - short name for the parser/command help argument. Can be left empty HelpLname - long name for the parser/command help argument. Leaving empty forces use of -h/--help when HelpDisabled is false NoExitOnHelp - defaults false, set true to not exit on help argument being parsed Should resolve all outstanding help argument issues.
Fixed merge conflict error due to check function return type change confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR, I found couple of small typos. Also please see review comment on having a single method NewCommand
instead of two.
Also note, that this decreases test coverage which currently fails the merge checks. Some newly added code is not covered by tests. |
I feel this is rather complex PR that adds unnecessary new methods to the parser (and commands). How do you feel instead of taking this approach to have a method that simply overrides default behavior. For example:
where function signature would be something like:
Also this PR shows a decrease in test coverage, which is set to fail merge checks. |
Seems like a reasonable solution, I'll adapt what I currently have and add code coverage to pass validation. |
Added functions: DisableHelp SetHelp ExitOnHelp Added Command.exitOnHelp Added more tests to increase code coverage
@akamensky Ready for review. I took your suggested SetHelpSettings functions and split it. Please let me know if anything needs changed. If this PR is satisfactory, it would solve issue #29 . |
Looks good. Merged. |
Finishes addressing issue. Removed check function to reduce repetitive code and enable extended help argument handling.